Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: check stream option and set encoding on fs.WritableStream #1412

Closed
wants to merge 2 commits into from
Closed

fs: check stream option and set encoding on fs.WritableStream #1412

wants to merge 2 commits into from

Conversation

yosuke-furukawa
Copy link
Member

Reviewer: @bnoordhuis

Problem 1

On Node.js v0.12

// illegal option but does not throw an Error, just ignore.
fs.createReadStream('foo.txt', 'utf8');

// illegal option but does not throw an Error, just ignore.
fs.createWriteStream('hoge', 'utf8');

On io.js v1.6.4

// illegal option and throw an Error
fs.createReadStream('foo.txt', 'utf8');

fs.js:1619
  options = Object.create(options || {});
                   ^
TypeError: Object prototype may only be an Object or null: utf8
    at Function.create (native)
    at new ReadStream (fs.js:1619:20)
    at Object.fs.createReadStream (fs.js:1608:10)
    at Object.<anonymous> (/Users/yosuke/go/src/github.com/yosuke-furukawa/fork/io.js/test/parallel/test-fs-write-stream-encoding.js:12:30)
    at Module._compile (module.js:410:26)
    at Object.Module._extensions..js (module.js:428:10)
    at Module.load (module.js:335:32)
    at Function.Module._load (module.js:290:12)
    at Function.Module.runMain (module.js:451:10)
    at startup (node.js:124:18)

// illegal option but does not throw an Error, just ignore.
fs.createWriteStream('hoge', 'utf8');

Bug found modules

Cause

I found this pull request #635 . this PR uses Object.create in createReadStream. Object.create does not accept string.

Of course, the above scripts and modules are weird. we have never written such options in our api document.

BUT I have 2 opinions.

  • the error message(TypeError: Object prototype may only be an Object or null: utf8) is not so easy to understand. We should return Bad Arguments like other functions.
  • fs.createReadStream and fs.createWriteStream should accept 2nd string argument as an encoding type. our existing functions like fs.readFile, fs.writeFile, have same behavior like here. the behavior misleads some developers.

Fix

Problem2

According to the createWriteStream api docs, encoding option could be found in the default.

But I set the encoding option to fs.createWriteStream('foo.txt', {encoding: 'utf8'}), the encoding option is ignored.

Fix

This pull request includes the fix both problem1 and problem2. comments and discussions are welcome.

@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. confirmed-bug Issues with confirmed bugs. labels Apr 13, 2015
cjihrig
cjihrig reviewed Apr 13, 2015
View reviewed changes
lib/fs.js
@@ -1615,6 +1615,11 @@ function ReadStream(path, options) {
if (!(this instanceof ReadStream))
return new ReadStream(path, options);

if (typeof options === 'string')
options = { encoding: options };
else if (options && typeof options !== 'object')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably should be more strict with this check. For example, options = 0 would slip through.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I will fix it.

@rvagg rvagg mentioned this pull request Apr 14, 2015
Closed
@bnoordhuis
Copy link
Member

LGTM. No strong opinion on the error message.

@yosuke-furukawa yosuke-furukawa added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 15, 2015
@yosuke-furukawa
Copy link
Member Author

@bnoordhuis @cjihrig I fixed!! Thank you for your review.

cjihrig
cjihrig reviewed Apr 15, 2015
View reviewed changes
lib/fs.js
@@ -1615,8 +1615,15 @@ function ReadStream(path, options) {
if (!(this instanceof ReadStream))
return new ReadStream(path, options);

if (options === undefined || options === null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably drop the null check, as it will work fine with Object.create().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@cjihrig
Copy link
Contributor

cjihrig commented Apr 15, 2015

FWIW, this can be interpreted as a breaking change - something was silently failing before could now throw an exception.

@yosuke-furukawa
Copy link
Member Author

Yes, but currently fs.createReadStream throws an exception if illegal option, fs.createWriteStream does not throw.
IMO that is not breaking change. We just fix the different behavior.

@yosuke-furukawa yosuke-furukawa mentioned this pull request Apr 16, 2015
Closed
@Fishrock123
Copy link
Contributor

If the "is breaking?" is controversial we can just tag it for semver-major and land it in the upcoming 2.0 anyways.

@yosuke-furukawa
Copy link
Member Author

We already break the compatible in fs.createReadStream .
The PR introduces this issue.
Should we revert it ??

@brendanashworth brendanashworth removed the confirmed-bug Issues with confirmed bugs. label Apr 21, 2015
@jonathanong
Copy link
Contributor

oh i didn't even know you could set options as a string. never saw that anywhere. i don't consider this "breaking" because 1) it's not documented and 2) it wasn't tested prior.

LGTM!

@jonathanong jonathanong mentioned this pull request Apr 21, 2015
Open
@yosuke-furukawa
Copy link
Member Author

Could I land it on master ??

@brendanashworth
Copy link
Contributor

+1 for patch, not major, because #635 got through on a patch - @yosuke-furukawa I think so.

@yosuke-furukawa
Copy link
Member Author

OK i will land this. Thanks for your remind @brendanashworth

This was referenced May 30, 2015
Closed
Closed
@brendanashworth
Copy link
Contributor

Closing in favor of the two new ones.

This was referenced Jun 15, 2015
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@yosuke-furukawa @bnoordhuis @cjihrig @Fishrock123 @jonathanong @brendanashworth @chrisdickinson @mscdex